-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable support for symlinks #510
Conversation
f4c22e0
to
d825513
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work @jeroenpf 🏅 !
I tested different scenarios and it worked as expected. I managed to symlink a local Gutenberg repository in a site, and I was able to debug and modify blocks on live 🎊.
The only issue I found is that seems the watcher to listen for file changes is not deallocated when stopping a site. Apart from this, the changes look good to me.
@fluiddot I've also solved the issue with the watcher not being stopped properly and also return a promise from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On macOS the PR is working great, however, symlinks are not working on Windows. Here's the results I'm getting:
How to create symlink
- Open Powershell session with admin privileges.
- Go to
<SITE_FOLDER>/wp-content/plugins
. - Run the command
New-Item -Path new-plugin.php -ItemType SymbolicLink -Value hello.php
.
Results
- When adding a symlink, this statement always return
false
. I investigated further and this is a problem with the path separator used. When usingpath.join
, the package picks up the one based on the OS, in this case, Windows. However, the PHP environment is Unix-based, so Windows paths won't work. - I forced using Unix separate when generating
vfsPath
, but the path returned when reading the symlink (ref) is returning a wrong value. E.g.vfsTarget = '/var/www/html/wp-content/plugins/C:\\Users\\Carlos\\Sstudio\\my-site\\wp-content\\plugins\\hello.php'
.
@jeroenpf I'm open to tackle this in a separate PR, in case we'd like to only provide support for symlinks in macOS. WDYT?
Good find! I will address it in this PR. We should be able to use |
I've added some tests and fixed some more small items based on feedback. |
@jeroenpf not a blocker but it would be great if we could solve the file conflicts with |
… commands (eg core update)
Add a symlink manager Remove old comments Fix issues and ensure that links are mounted on instance rotation Fixes for abortController Revert unnecessary change Update vendor/wp-now/src/symlink-manager.ts Co-authored-by: Carlos Garcia <fluiddot@gmail.com> Update vendor/wp-now/src/symlink-manager.ts Co-authored-by: Carlos Garcia <fluiddot@gmail.com> capitalize type name Init maps in constructor Always remove symlink from map on removal Exit php when server is stopped Refactor the symlink manager and move it to the lib directory Small fixes Force usage of POSIX compliant paths when dealing with VFS paths Prevent loading symlink manager on windows platforms Add unit tests for symlink creation/deletion Remove unused import Specifically check for win32 before using SymlinkManager
72c8c8e
to
052bdd1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎊 ! Awesome work @jeroenpf 🏅 !
I've tested the functionality on different platforms:
- macOS ✅
- Windows ✅ (Symlinks are disabled)
- Linux ✅
I've added some comments in the unit tests, but none should block merging the PR.
Co-authored-by: Carlos Garcia <fluiddot@gmail.com>
Co-authored-by: Carlos Garcia <fluiddot@gmail.com>
Co-authored-by: Carlos Garcia <fluiddot@gmail.com>
Co-authored-by: Carlos Garcia <fluiddot@gmail.com>
Related to https://github.com/Automattic/dotcom-forge/issues/8817
Proposed Changes
This PR aims to ensure that symlinks found inside of a site directory are working. We do this by first scanning and later watching the file system for symlinks and then creating mounts for the targets of each found symlink.
Since symlink targets inside of the NODEFS are not always predictable, we need to get the actual target of the link as seen by the FS inside the PHP runtime. When we determine the correct target, we create a mount point that points to the
realpath
of the symlink on the host system, essentially bypassing the need for chained links. (e.g.link -> link 2 -> target
)We also need to ensure that we keep a reference count of how many links point to a specific mounted target. It is possible that multiple symlinks point to the same target. As we are trying to unmount unnecessary targets, we need to be sure no other links point to it when we attempt to delete it.
Additionally, we need to ensure that we do not delete pre-existing files or directories at the target path. E.g. if a target already existed before we found a link pointing to it, we never want to remove it if the symlinks are removed.
Potential issue:
I found that deleting a symlink correctly removes files and links from the NODESF but when trying to load it via the browser, it throws an error of file not found. I think this might be related to how PHP or the underlying server is caching things or perhaps something related to the NODEFS. We might need to investigate that at some point.
Testing Instructions
<?php echo time();
in that file.ln -s ~/your-file.php test.php
- this creates a symlinktest.php
that points to a file you created elsewhere.http://localhost:<port>/test.php
- you should see the current timestamp or the output of whatever you placed in the file.Extra scenarios to test:
wp-content/plugins
to that plugin and see if it appears in wp-admin.Pre-merge Checklist